feat: svg sprite generation #1
Conversation
Adds SVG sprite generation to customize video.js icons - The `bin` folder contains the `cli.js` file for command-line usage. - The `icons` folder contains all the SVG icons used to generate the sprite. - The `src` folder contains the `svg-sprite` and `SVGO` configuration files, as well as the script for generating the sprite. - The `template` folder includes the HTML file used for the preview page, along with a configuration file that can be generated via the CLI. - The `vjs-svg-sprite` folder contains the preview page and the default sprite. - The `index.js` file is only used for development and generating the default sprite. - The `svg-icons.json` file defines the default configuration.
|
Great stuff, a couple of quick thoughts
|
|
@mister-ben I've added the .nvmrc file.
Yes, because I've applied two optimizations, the first directly to the icons and the second directly to the generated sprite, which should further reduce the size of the final sprite. Individual icon optimization svg-sprite-generator/src/vjs-sprite.js Line 41 in f9b1014 Sprite optimization svg-sprite-generator/src/vjs-sprite.js Line 53 in f9b1014
Yes, I can also change the destination filename to |
Regenerate `vjs-svg-sprite/index.html` to remove reference to wrong repo.
From now on, the value of `output-dir` is `dist` instead of `vjs-svg-sprite`. Access to the SVG file will be via the path `@videojs/svg-sprite-generator/dist/vjs-sprite-icons.svg` instead of `@videojs/svg-sprite-generator/vjs-svg-sprite/vjs-sprite-icons.svg`.
There was a problem hiding this comment.
This looks great, nice work! I left a few comments and questions.
Regarding the usage of this repo, is the distributed svg file meant to replace the file here? https://github.com/videojs/video.js/pull/8260/files#diff-56a92727a19631fea08e4d8424bbf11cbf939176723287f3e2947eded506fc83
LICENSE
Outdated
| @@ -0,0 +1,21 @@ | |||
| MIT License | |||
|
|
|||
| Copyright (c) 2024 amtins<amtins.dev@gmail.com>. | |||
There was a problem hiding this comment.
We may want to update this file to align with https://github.com/videojs/video.js/blob/main/LICENSE
| @@ -0,0 +1,2 @@ | |||
| node_modules | |||
| vjs-sprite-tmp_* | |||
There was a problem hiding this comment.
Do we want to include dist in this file?
There was a problem hiding this comment.
Ideally, yes, in order to replace the player.js import with the file generated by this project. A bit like videojs-icons.css
| params: { | ||
| attributes: [ | ||
| { | ||
| style: 'display: none' |
There was a problem hiding this comment.
I am sure there is good reason, but why do we want to set this style on clean?
There was a problem hiding this comment.
The main reason is that svg-sprite adds style="position:absolute". So the cleanup is to replace position:absolute with display:none. Currently, display:none is applied via JavaScript and is not present in the icons.svg file. This would allow to remove line 545.
template/svg.config.file.js
Outdated
| "root-dir": "", | ||
| "output-dir": "", | ||
| "icons": { | ||
| "audio-description": "", |
There was a problem hiding this comment.
This seems bit repetitive as a similar structure exists in scg-icons.json. Would it be possible to just move those values here and remove the other file?
It may not be possible as I am unfamiliar with svg-sprite, but figured I would mention it and try to get a better understanding.
There was a problem hiding this comment.
The lack of a README adds some confusion. To give a bit of context, there are two aspects to this project:
- Standardize and simplify the generation of the SVG sprite used by video.js. This would allow us to remove the
icons.svgfile and replace the import with the file generated by this project. I was inspired by the same philosophy as the font project, which is why this project also provides the final file. So thesvg-icons.jsonfile is used to generate the sprite for this project. - Allow developers to easily replace the default icons used by video.js. All of this can be done using the command-line tool. Therefore,
svg.config.file.jswill serve as a configuration file template for developers to customize the icons.
| "vjs-svg-sprite": "./bin/cli.js" | ||
| }, | ||
| "scripts": { | ||
| "build": "node index.js", |
There was a problem hiding this comment.
I am trying to think if there are any other scripts we may want to include here.. Would we want to include lint, or maybe clean to remove the dist?
There was a problem hiding this comment.
I have added the linter as well as husky.
Thank you for taking the time to review this PR and for the comments. ❤️
I am open to any other suggestions or ideas you may have.
Run linter on pre-commit hook
The svg-sprite.config.js file is ignored because import.meta.url is not compatible with the version of javascript used by vjsstandard.
Regenerate sprite file and html page.
Description
Adds SVG sprite generation to customize video.js icons.
Specific Changes proposed
binfolder contains thecli.jsfile for command-line usage.iconsfolder contains all the SVG icons used to generate the sprite.srcfolder contains thesvg-spriteandSVGOconfiguration files, as well as the script for generating the sprite.templatefolder includes the HTML file used for the preview page, along with a configuration file that can be generated via the CLI.vjs-svg-spritefolder contains the preview page and the default sprite.index.jsfile is only used for development and generating the default sprite.svg-icons.jsonfile defines the default configuration.